Skip to content
This repository has been archived by the owner on Jan 23, 2023. It is now read-only.
/ corefx Public archive

Update and addition to Performance tests #3764

Merged
merged 1 commit into from
Oct 14, 2015
Merged

Update and addition to Performance tests #3764

merged 1 commit into from
Oct 14, 2015

Conversation

ianhays
Copy link
Contributor

@ianhays ianhays commented Oct 10, 2015

  • Made the existing perf tests for Linq follow the new syntax and use the new runner
  • Removed all references to v5.0 libraries being required as they no longer are
  • Added new tests using usage data from aspnet/benchmarks
  • Modified some existing tests to be more easily comparable using analysis tools that look at inlinedata/memberdata (e.g. System.IO.Compression)
  • Updated the lock files to use the latest xunit-performance package
  • Modified perf tests for System.Diagnostics.Process to provide more consistent results.

@ianhays ianhays added tenet-performance Performance related issue X-Plat labels Oct 10, 2015
@ianhays ianhays self-assigned this Oct 10, 2015
@ianhays ianhays added this to the 1.0.0-rc1 milestone Oct 10, 2015
@@ -8,7 +8,7 @@
using Xunit;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The SocketPerformanceTest changes shouldnt be in here. I'm going to put those tests in a separate PR shortly.

@ianhays
Copy link
Contributor Author

ianhays commented Oct 10, 2015

@stephentoub

@ianhays
Copy link
Contributor Author

ianhays commented Oct 10, 2015

Depends on dotnet/buildtools#308

using (iteration.StartMeasurement())
for (int i = 0; i < 10000; i++)
{
list.Capacity = size + 1;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is measuring the performance of Capacity when it doesn't actually need to change the size of the list, since it's always being set to the same value. Is that intentional?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The docs here only mention that setting the capacity is O(n) where n is the new size. They did not mention the special case of if the new capacity == the old capacity, but thinking on it now there's no reason to reallocate a new internal array if the desired size is already met with the old array. I'll modify this test to set the capacity to a different value each iteration.

@stephentoub
Copy link
Member

Left some comments, but generally looks good.

namespace System.Numerics.Tests
{
// Equals, DotProduct, ctor
public class Perf_Vector
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The most interesting things to test here are probably the methods under the "Intrinsics" files. Those are operations that are specially handled by the JIT. The most interesting ones are probably:

  • Operator +, Operator -, Operator *, and the named versions, Add, Subtract, Multiply
  • Dot()
  • SquareRoot
  • Length(Squared)
  • Normalize
  • Cross (for Vector3)
  • Distance(Squared)

I'd also like to have them separated out by type (i.e. Vector2, Vector3, Vector4), because the code generated for them is probably different enough that we want to separate the results.

@stephentoub
Copy link
Member

@dotnet-bot test this please (after Jenkins changes related to setting culture-related env vars)

@ianhays
Copy link
Contributor Author

ianhays commented Oct 13, 2015

@mellinoe added some new tests for Vector, separated into Vector2, 3, 4 like you wanted :)

@stephentoub responded to all of your feedbacks. Thanks for the great comments.

I still need to:

  • remove sockets changes
  • remove stringcultureinfo tests
  • Update buildtools PR and get that merged

@mellinoe
Copy link
Contributor

@ianhays The vector tests look good overall to me. My only concern is that the test structure might have a non-negligible effect on the results. I think the method call to ExecuteTest and then the switch jump might add some overhead that could affect results, since the vector operations themselves often get JIT'd down to a single instruction, or just a few instructions. Overall I think it's probably small enough of an overhead that I am not too concerned.

Anyways, I think they are fine to merge right now, the content of the tests looks good to me and should give accurate enough results. When I have some time I may go through them and implement them using T4 templates to avoid the two points of overhead above. That way, we could basically move all of the switching/method calling to code-generation time, and at run-time only execute the inner benchmark logic.

@ianhays
Copy link
Contributor Author

ianhays commented Oct 13, 2015

Thanks for the review and direction for the tests Eric!

My only concern is that the test structure might have a non-negligible effect on the results.

The call and switch have no appreciable effect on the results when the number of innerIterations as high as I set it (1000000). The innerIterations loop, however, adds roughly .3 ms to the test time. For the Vector2 tests for Add_operator, sub_operator, mul_operator, Dot, SquareRoot, and Length_Squared this inner loop makes up the entirety of the test timing. The best solution to this would be a massive amount of loop unrolling, but I agree with you that templating may be a better answer. While accuracy is absolutely the primary concern, I don't want to sacrifice too much in succinctness if we can avoid it with these microbenchmarks.

Either way, the results for Linux vs Windows are far enough off that the switch overhead is a low priority for the time being. I'll post an info dump with the results today :)

- Made the existing perf tests for Linq follow the new syntax and use the new runner
- Removed all references to v5.0 libraries being required as they no longer are
- Added new tests using usage data from aspnet/benchmarks
- Modified some existing tests to be more easily comparable using analysis tools that look at inlinedata/memberdata (e.g. System.IO.Compression)
- Updated the lock files to use the latest xunit-performance package
- Modified test structure and innerIteration loops to produce more consistently repeatable results
- Updated docs with change to csproj requirements
@ianhays
Copy link
Contributor Author

ianhays commented Oct 13, 2015

Rebased and moved around a few numbers.

@stephentoub
Copy link
Member

LGTM. Thanks.

stephentoub added a commit that referenced this pull request Oct 14, 2015
Update and addition to Performance tests
@stephentoub stephentoub merged commit 8fc93d4 into dotnet:master Oct 14, 2015
@ianhays ianhays deleted the perf branch October 14, 2015 15:48
@karelz karelz added the os-linux Linux OS (any supported distro) label Mar 8, 2017
picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022
Update and addition to Performance tests

Commit migrated from dotnet/corefx@8fc93d4
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
os-linux Linux OS (any supported distro) tenet-performance Performance related issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants